I was able to reproduce locally the issues from #39515 and #41081 with the reproducers provided by the Quarkus users. Then, I confirmed that this PR fixes both issues.

The Vert.x context is untested in quarkus-cache though. I'm trying to determine if that can be tested somehow.

@geoand I'm out of time for today but I'll try to add the test later with a separate PR. Until then, I think this PR can be merged. WDYT?

Yeah, I agree.

Let's also get a +1 from @cescoffier

Hm... I'm wondering if this change would affect Infinispan and Redis cache implementations...

The Infinispan impl depends on a separate CacheResultInterceptor and does not depend on CaffeineCacheImpl so I think we're safe here. I'm still checking why the test failed...

I'm not sure about the Redis impl.

The Infinispan impl depends on a separate CacheResultInterceptor and does not depend on CaffeineCacheImpl so I think we're safe here. I'm still checking why the test failed...

I think this is only the case for io.quarkus.infinispan.client.CacheResult - cc @karesti

I'm not sure about the Redis impl.

cc @cescoffier

The test from quarkus-infinispan-cache that failed depends on the annotation and interceptor from quarkus-cache while a variation of both the annotation and the interceptor exist in quarkus-infinispan-client. I'm a bit confused about how things are organized in the Infinispan impl today. I've been away from too long 😅

The test from quarkus-infinispan-cache that failed depends on the annotation and interceptor from quarkus-cache while a variation of both the annotation and the interceptor exist in quarkus-infinispan-client. I'm a bit confused about how things are organized in the Infinispan impl today. I've been away from too long 😅

Custom annotations are deprecated in the infinispan client extension, they were introduced before the SPI existed. The infinispan cache extension is now fully providing the replacement of caffeine by infinispan with the new extension

@geoand @gwenneg @cescoffier I don't know if we need any fix on the deprecated annotations, since the new extension provides the fix . what do you think ?

The test from quarkus-infinispan-cache that failed depends on the annotation and interceptor from quarkus-cache while a variation of both the annotation and the interceptor exist in quarkus-infinispan-client. I'm a bit confused about how things are organized in the Infinispan impl today. I've been away from too long 😅

https://quarkus.io/guides/cache-infinispan-reference
https://www.youtube.com/watch?v=y1YR57_p2Yo

I don't think the old annotations need a fix, but we do need to make sure that @gwenneg's fix doesn't break Infinispan with the regular cache annotations (my guess is that it might, as I don't see the context juggling that was moved to Caffeine)

It needs a proper tests but I think the Redis cache is going to propagate the requester duplicated context, and the callback from Redis is going to preserve the duplicated context.

@geoand how should I be testing this to make sure, beyond the already existing tests

Take a look at the issues this PR closes.
You would likely be able to reproduce the same problems with the infinispan cache backend.
My assumption is that this PR will not make things any better for Infinispan, if anything it should make it worse

@geoand do you think I should move the change to the cache impl ? I can have a closer look tomorrow

I think the infinispan cache implementation will need a similar change to this one

@gwenneg I opened a PR here https://github.com/gwenneg/quarkus/pull/298/files

@gwenneg could you please merge (without a merge commit) Katia's PR into your branch so this PR can be updated?

Thanks

Sure, done!

Thanks a lot!

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit dca2139.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

Hi @gwenneg. Did you have chance to look adding the test later with a separate PR as noted in #41647 (comment)?

Hey @rsvoboda. No, not yet. I'm currently on PTO so that will have to wait a bit.